Skip to content

Conversation

@PaurushGarg
Copy link
Contributor

@PaurushGarg PaurushGarg commented Dec 17, 2025

What this PR does:
Adds Native Histogram extraction logic in Results Cache.

Which issue(s) this PR fixes:
Currently NH extraction is missing in Extract in Result Cache. This produces incorrect results when cache is hit for queries like raw histogram queries (or with aggregation like sum).

Fixes #

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: Paurush Garg <paurushg@amazon.com>
@PaurushGarg PaurushGarg force-pushed the results-cache-extract-histogram branch from d12c42e to 2246222 Compare December 17, 2025 17:54
Copy link
Contributor

@justinjung04 justinjung04 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thank you

Copy link
Member

@SungJin1212 SungJin1212 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Dec 18, 2025
Copy link
Contributor

@afhassan afhassan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

}
}
if len(result.Samples) == 0 {
if stream.Histograms != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I don't think we need to check for nil here since both Samples and Histograms are not nullable

repeated cortexpb.Sample samples = 2 [(gogoproto.nullable) = false, (gogoproto.jsontag) = "values"];
repeated SampleHistogramPair histograms = 3 [(gogoproto.nullable) = false, (gogoproto.jsontag) = "histograms"];

@yeya24 yeya24 merged commit e51d5a2 into cortexproject:master Dec 18, 2025
25 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/querier lgtm This PR has been approved by a maintainer size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants